Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

r/wafv2_web_acl_logging_configuration: update redacted_fields schema definition #14319

Merged
merged 7 commits into from
Mar 18, 2021
Merged

r/wafv2_web_acl_logging_configuration: update redacted_fields schema definition #14319

merged 7 commits into from
Mar 18, 2021

Conversation

anGie44
Copy link
Contributor

@anGie44 anGie44 commented Jul 23, 2020 •
edited
Loading

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #14248
Closes #14244

Notes:

The linked GH-Issues brought to light the following:

  • redacted_fields blocks in the context of a LoggingConfiguration only permits a subset of the fields documented in https://docs.aws.amazon.com/waf/latest/APIReference/API_FieldToMatch.html and the API will error when an unsupported field (e.g. single_query_argument) is provided --> to account for this behavior, a custom schema is made for this argument instead of using the shared one in waf_helper.go; fields that are not supported have been marked as Deprecated until they can officially be removed

  • only 1 nested attribute is valid in the redacted_fields configuration block of aws_wafv2_web_acl_logging_configuration and the field_to_match block in the wafv2_web_acl and wafv2_rule_group resources --> to account for this, the documentation has been updated to prevent users from setting multiple attributes

  • the previous Set type used for redacted_fields would be evaluated as an empty object when fields with empty schemas (e.g. body) were configured --> to address this, the field argument has been changed to type List

Release note for CHANGELOG:

resource/wafv2_logging_configuration: update redacted_fields handling

Output from acceptance testing:

🏃‍♀️ 🏃‍♂️ 🏃‍♀️ 🏃‍♂️ sooo fast, terraform 0.14 💪  
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears (88.40s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_webACLDisappears (97.98s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateQueryStringRedactedField (102.55s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateEmptyRedactedFields (118.02s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateUriPathRedactedField (123.19s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_emptyRedactedFields (124.89s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_basic (132.33s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMethodRedactedField (140.09s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMultipleRedactedFields (147.57s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateSingleHeaderRedactedField (154.22s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeResourceARNForceNew (199.44s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeLogDestinationConfigsForceNew (200.19s)

@anGie44 anGie44 requested a review from a team July 23, 2020 14:34
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. labels Jul 23, 2020
@anGie44 anGie44 added bug Addresses a defect in current functionality. service/wafv2 Issues and PRs that pertain to the wafv2 service. labels Jul 23, 2020
@anGie44 anGie44 marked this pull request as draft July 23, 2020 15:45
@ghost ghost added the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Jul 23, 2020
@anGie44 anGie44 mentioned this pull request Jul 23, 2020
Closed
@anGie44 anGie44 marked this pull request as ready for review July 23, 2020 16:49
@anGie44 anGie44 changed the title service/wafv2: update field_to_match and error handling service/wafv2: update redacted_fields and field_to_match usage and add error handling Jul 23, 2020
@anGie44 anGie44 force-pushed the b-wafv2-logging-config branch 2 times, most recently from 94c078d to fb9a9ac Compare July 24, 2020 05:26
@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Jul 24, 2020
@anGie44 anGie44 force-pushed the b-wafv2-logging-config branch 2 times, most recently from 3d7c349 to c7429ef Compare August 8, 2020 19:58
@bflad
Copy link
Contributor

bflad commented Nov 9, 2020

@anGie44 can you please rebase this pull request?

@anGie44 anGie44 requested a review from a team as a code owner November 9, 2020 18:35
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Nov 9, 2020
anGie44
anGie44 commented Nov 9, 2020
View reviewed changes
aws/resource_aws_wafv2_web_acl_logging_configuration.go
// as they are not supported by the WAFv2 API
// in the context of WebACL Logging Configurations
// Reference: https://docs.aws.amazon.com/waf/latest/APIReference/API_LoggingConfiguration.html (RedactedFields)
"all_query_arguments": wafv2EmptySchemaDeprecated(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can skip this deprecation part (since they don't function as is?)

@anGie44 anGie44 force-pushed the b-wafv2-logging-config branch 2 times, most recently from 662d162 to c3fdbfb Compare December 3, 2020 16:46
@anGie44 anGie44 marked this pull request as ready for review December 3, 2020 17:41
anGie44
anGie44 commented Dec 3, 2020
View reviewed changes
aws/resource_aws_wafv2_web_acl_logging_configuration.go Outdated
// (e.g. body {}) will result in a nil redacted_fields attribute
Type: schema.TypeList,
Optional: true,
DiffSuppressFunc: suppressRedactedFieldsDiff,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understandably not an ideal side-effect :/ any thoughts on using a different hack to work around the order diffs?

@anGie44 anGie44 changed the title service/wafv2: update redacted_fields and field_to_match usage and add error handling r/wafv2_weba_acl_logging_configuration: update redacted_fields and field_to_match usage and add error handling Dec 3, 2020
@anGie44 anGie44 changed the title r/wafv2_weba_acl_logging_configuration: update redacted_fields and field_to_match usage and add error handling r/wafv2_web_acl_logging_configuration: update redacted_fields and field_to_match usage and add error handling Dec 3, 2020
@anGie44 anGie44 changed the title r/wafv2_web_acl_logging_configuration: update redacted_fields and field_to_match usage and add error handling r/wafv2_web_acl_logging_configuration: update redacted_fields schema definition Dec 9, 2020
Base automatically changed from master to main January 23, 2021 00:58
@gdavison gdavison self-assigned this Feb 16, 2021
gdavison
gdavison approved these changes Feb 17, 2021
View reviewed changes
Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀

Waiting on the acceptance tests

gdavison
gdavison approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acceptance tests

--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateUriPathRedactedField (99.34s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears (116.45s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_emptyRedactedFields (116.61s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_basic (120.68s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMultipleRedactedFields (125.83s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears_WebAcl (129.62s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateQueryStringRedactedField (135.83s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateEmptyRedactedFields (138.26s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateSingleHeaderRedactedField (149.56s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMethodRedactedField (150.68s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeLogDestinationConfigsForceNew (182.36s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeResourceARNForceNew (200.66s)

@denis-khalturin-incountry
Copy link

@gdavison, are we awaiting a second review for a PR merge?

@anGie44 anGie44 added this to the v3.33.0 milestone Mar 18, 2021
@anGie44
Copy link
Contributor Author

anGie44 commented Mar 18, 2021

Hi @denis-khalturin-incountry , this one slipped my radar! Waiting on CI checks and then will merge 👍

@anGie44
Copy link
Contributor Author

anGie44 commented Mar 18, 2021

Output of acceptance tests (commercial):

--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears (104.02s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_basic (111.54s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMethodRedactedField (138.00s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMultipleRedactedFields (144.81s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateEmptyRedactedFields (147.75s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateQueryStringRedactedField (150.65s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears_WebAcl (152.47s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_emptyRedactedFields (153.96s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateUriPathRedactedField (162.43s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateSingleHeaderRedactedField (179.40s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeLogDestinationConfigsForceNew (198.54s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeResourceARNForceNew (238.30s)

gov cloud:

--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_emptyRedactedFields (70.81s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears_WebAcl (73.56s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateQueryStringRedactedField (99.73s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_disappears (100.06s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_basic (107.22s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateEmptyRedactedFields (107.70s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateUriPathRedactedField (109.45s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMethodRedactedField (121.89s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateSingleHeaderRedactedField (124.12s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_updateMultipleRedactedFields (124.39s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeLogDestinationConfigsForceNew (146.54s)
--- PASS: TestAccAwsWafv2WebACLLoggingConfiguration_changeResourceARNForceNew (180.79s)

@anGie44 anGie44 merged commit de09978 into main Mar 18, 2021
@anGie44 anGie44 deleted the b-wafv2-logging-config branch March 18, 2021 18:35
github-actions bot pushed a commit that referenced this pull request Mar 18, 2021
Update CHANGELOG.md for #14319
9029bcf
@ghost
Copy link

ghost commented Mar 18, 2021

This has been released in version 3.33.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@kamatama41 kamatama41 mentioned this pull request Mar 24, 2021
Closed
@ghost
Copy link

ghost commented Apr 18, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dennis-grunert dennis-grunert dennis-grunert left review comments

@gdavison gdavison gdavison approved these changes

Assignees

@gdavison gdavison

Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. service/wafv2 Issues and PRs that pertain to the wafv2 service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v3.33.0
5 participants
@anGie44 @bflad @denis-khalturin-incountry @gdavison @dennis-grunert